Skip to content

Avoid useless initializations of fci/fcc in array functions #18273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Apr 7, 2025

These cause cache misses due to global access, in phpstan (notably the array_map).
Initializing these isn't necessary because ZPP initializes it for us. Only for optional arguments do we need to be careful; for array_filter we still reset the fci but not fci_cache because fci is not necessarily set by ZPP but is conditionally used to access fci_cache.

These cause cache misses due to global access, in phpstan
(notably the array_map).
Initializing these isn't necessary because ZPP initializes it for us.
Only for optional arguments do we need to be careful; for `array_filter`
we still reset the `fci` but not `fci_cache` because `fci` is not
necessarily set by ZPP but is conditionally used to access `fci_cache`.
@nielsdos nielsdos marked this pull request as ready for review April 8, 2025 20:35
@nielsdos nielsdos requested a review from bukka as a code owner April 8, 2025 20:35
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Apr 14, 2025
See php#18273, a constant may cause
unnecessary cache misses.
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized I never actually approved this. I checked all the cases again and this looks correct to me.

iluuu1994 added a commit that referenced this pull request Apr 15, 2025
See #18273, a constant may cause
unnecessary cache misses.

Closes GH-18326
@nielsdos nielsdos merged commit ea387fc into php:master Apr 15, 2025
9 of 17 checks passed
@staabm
Copy link
Contributor

staabm commented Apr 18, 2025

I guess its related to this PR, therefore posting it here.

running phpstan/phpstan-src@b3d3386 on macOS considerable got faster:
maybe also affects other OS, but didn't test


running 8376904 with

php bin/phpstan clear-result-cache -q; time /Users/staabm/workspace/php-src/sapi/cli/php -d memory_limit=448M bin/phpstan

3 runs runtime:

130.12s user 4.62s system 779% cpu 17.275 total
130.53s user 4.63s system 769% cpu 17.570 total
126.24s user 4.24s system 732% cpu 17.825 total

vs.


running 1684c52a88d with the same command:

3 runs runtime:

146.76s user 4.63s system 793% cpu 19.075 total
146.02s user 4.62s system 778% cpu 19.340 total
145.92s user 4.64s system 773% cpu 19.455 total

vs. PHP 8.3.20 (cli) (built: Apr 8 2025 20:21:18) (NTS)

3 runs runtime:

145.78s user 4.77s system 763% cpu 19.710 total
147.19s user 5.02s system 767% cpu 19.835 total
146.75s user 4.89s system 773% cpu 19.613 total

vs. PHP 8.4.5 (cli) (built: Mar 12 2025 01:55:56) (NTS)

3 runs runtime:

147.12s user 4.63s system 770% cpu 19.698 total
150.61s user 4.91s system 781% cpu 19.894 total
147.29s user 4.71s system 777% cpu 19.560 total

@nielsdos
Copy link
Member Author

At least also because of the CALL VM optimization.

@iluuu1994
Copy link
Member

Indeed. @staabm This is more likely related to #17952, which will specifically improve macOS (or rather compilation with Clang), but not much otherwise I believe.

@staabm
Copy link
Contributor

staabm commented Apr 18, 2025

Thanks to anyone involved 😎

//cc @arnaud-lb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants